Skip to content

fix(watch-later): inject button outside aria-hidden anchor (#4305) - #4307

Open
shoaibyazdani wants to merge 2 commits into
code-charity:masterfrom
shoaibyazdani:fix/watch-later-aria-hidden
Open

fix(watch-later): inject button outside aria-hidden anchor (#4305)#4307
shoaibyazdani wants to merge 2 commits into
code-charity:masterfrom
shoaibyazdani:fix/watch-later-aria-hidden

Conversation

@shoaibyazdani

Copy link
Copy Markdown

Summary

Fixes #4305.

YouTube marks thumbnail anchors (e.g. <a class="ytLockupViewModelContentImage">) with aria-hidden="true". The Watch Later button was being appended as a direct child of the anchor via thumbnail.appendChild(button), so when a user clicked the button the browser blocked focus with:

Blocked aria-hidden on an element because its descendant retained focus. The focus must not be hidden from assistive technology users.

Appending the button to the thumbnail's parent container (the renderer — ytd-rich-item-renderer / yt-lockup-view-model / etc.) keeps the button focusable while preserving YouTube's a11y semantics.

Why this works

  • The button uses position: absolute; top: 4px; right: 4px; and is already positioned relative to a higher ancestor (the renderer). Moving it from the anchor to the renderer's child list doesn't change visual placement.
  • findNativeWatchLaterButton(container) works correctly because that helper uses closest() to locate the renderer internally — passing the renderer directly just short-circuits the lookup.
  • always mode is unaffected. The existing *:hover>.it-watch-later-button selector still matches because the button's direct parent (the renderer) is now what gets hovered — so hover semantics still work, just with a slightly larger hover area. If reviewers prefer to preserve link-only hover, the selector can be tightened in a follow-up.

Changes

  • js&css/extension/www.youtube.com/general/general.jsaddWatchLaterButton: append to thumbnail.parentElement instead of thumbnail; track container so the click handler can pass it to findNativeWatchLaterButton (which uses closest() anyway, but passing the renderer explicitly is cleaner).
  • tests/unit/watch-later-buttons.test.js — added a test asserting the button is not appended directly to the thumbnail anchor.

Test

PASS tests/unit/watch-later-buttons.test.js
  Watch Later thumbnail buttons
    ✓ registers the feature with init
    ✓ adds a hover and always menu option
    ✓ uses the native Watch Later control before the Innertube fallback
    ✓ styles hover and always visibility states
    ✓ injects button outside the aria-hidden thumbnail anchor (#4305)

Tests:       5 passed, 5 total

Notes

…ity#4305)

YouTube marks thumbnail anchors (e.g. <a class="ytLockupViewModelContentImage">) with aria-hidden="true". The Watch Later button was appended as a direct child of the anchor, so clicking the button triggered the browser's focus-block warning:

  Blocked aria-hidden on an element because its descendant retained focus.

Append the button to the thumbnail's parent container (the renderer — ytd-rich-item-renderer, yt-lockup-view-model, etc.) instead. The button is positioned absolutely so visual placement is unchanged.

- addWatchLaterButton: append to thumbnail.parentElement; track container for the click handler so it can be passed to findNativeWatchLaterButton (which uses closest() to locate the renderer internally anyway).
- findNativeWatchLaterButton(container) still resolves the same renderer via closest(), so this is a no-op for the native-button lookup.
- Existing hover selector *:hover>.it-watch-later-button still matches because the button's new direct parent (the renderer) is what gets hovered.
- New test: 'injects button outside the aria-hidden thumbnail anchor (code-charity#4305)'.

@wahajahmed010 wahajahmed010 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch on the a11y issue — the explanation is clear and the diff is minimal. Two concerns:

1. The new test is a static string assertion, not a behavioral test

expect(generalJs).not.toMatch(/thumbnail\.appendChild\s*\(\s*button\s*\)/);
expect(generalJs).toMatch(/(?:container|parentElement)\.appendChild\s*\(\s*button\s*\)/);

This passes as long as the source contains those patterns somewhere — it doesn't verify the function actually appends to the right element at runtime. For example, if someone refactors addWatchLaterButton to a new helper but accidentally leaves a stale thumbnail.appendChild(button) in a comment or fallback branch, the test still passes. Consider asserting against the rendered DOM instead:

test('appends watch-later button to parent container, not thumbnail', () => {
  document.body.innerHTML = `
    <ytd-rich-item-renderer>
      <a class="ytLockupViewModelContentImage" aria-hidden="true" href="/watch?v=abc"></a>
    </ytd-rich-item-renderer>`;
  const renderer = document.querySelector('ytd-rich-item-renderer');
  const thumbnail = renderer.querySelector('a');
  addWatchLaterButton(thumbnail);
  expect(thumbnail.querySelector('button')).toBeNull(); // not direct child
  expect(renderer.querySelector('button')).not.toBeNull(); // sibling-ish
});

(Adjust to your actual test harness — but the principle is the same: assert behavior, not source.)

2. Edge case: thumbnail.parentElement === null

container = thumbnail ? thumbnail.parentElement : null and the early-return if (... && container && ...) correctly guards against null. But parentElement is only null when thumbnail is the document root, which won't happen for YouTube renderers in practice. Worth either:

  • A short code comment documenting the assumption, or
  • A test that asserts graceful behavior when thumbnail.parentElement is null.

3. Minor: hover semantics change

The author notes the hover area now extends to the entire renderer rather than just the link. If this regresses visual behavior (button appearing on areas that shouldn't trigger it), consider tightening the CSS selector as suggested in the PR body. Otherwise it's a fine trade-off.

The fix itself is correct and the surgical scope is appreciated. Lgtm modulo the test-quality concern.

…ertions (code-charity#4305)

Replaces the regex-on-source test Wahaj flagged with three jsdom-based
tests that actually verify behavior against the rendered DOM:

- appends the button to the parent container, not the aria-hidden anchor
  (the core a11y fix — catches regressions)
- does not duplicate the button on repeat calls (idempotency)
- skips a thumbnail without a video id

The behavioral tests extract addWatchLaterButton (+ its in-closure
dependencies getVideoId and findNativeWatchLaterButton) from general.js
and eval them in a jsdom context so the real function runs against a
real DOM, not just source-text patterns.

Adds jsdom@^22.1.0 as a devDependency (22.x is the last pre-ESM-deps
release line that works cleanly with Jest 29).
@shoaibyazdani

Copy link
Copy Markdown
Author

Good call on the static-string test — replaced it with three behavioral tests that run the actual addWatchLaterButton function against a real DOM (jsdom) and assert on the rendered output, not source patterns:

  • appends the button to the parent container, not the aria-hidden anchor — the core a11y fix. Calls the real function and checks thumbnail.children.length === 0 + the button is a direct child of the renderer.
  • does not duplicate the button on repeat calls — idempotency guard. Calls the function twice on the same thumbnail and asserts only one button ends up in the DOM.
  • skips a thumbnail without a video id — early-return guard. Passes a non-watch URL and asserts nothing gets appended.

The functions are extracted from general.js via regex (they're nested inside the watchLaterButtons closure) and eval'd in a jsdom context with document bound — so we're exercising the real code path, not a copy.

Added jsdom@^22.1.0 as a devDep (22.x is the last line that's free of ESM-only transitive deps that Jest 29 can't transform out of the box).

CI status on this push will tell us if the jsdom install plays nicely with your existing runner setup. Happy to pin differently or move the tests to a separate suite if you'd prefer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🐞Watch Later button throws error

2 participants